Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Describe Rails subscriber implementation #2179

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

hannahramadan
Copy link
Contributor

Adds a code comment for future readers. The agent uses a non-traditional way to subscribe to Rails events. Instead of passing a block to ActiveSupport::Notifications.subscribe, the agent defines a #start and #finish method, which Rails responds to. This is due to a threading issue discovered on initial instrumentation.

The agent uses a non-traditional way to subscribe to Rails events.
Instead of passing a block to ActiveSupport::Notifications.subscribe, the
agent defines a #start and #finish method, which Rails responds to.
This is due to a threading issue discovered on initial instrumentation.
Issue: rails/rails#12069
Rails code: https://github.com/rails/rails/blob/ed5af004598fa7645fec2210453cca00f4b59168/activesupport/lib/active_support/notifications/fanout.rb#L320
@hannahramadan hannahramadan marked this pull request as ready for review August 30, 2023 00:03
kaylareopelle
kaylareopelle previously approved these changes Aug 30, 2023
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this addition is great as-is. The only change I'd recommend is to add a link to the Rails issue in the comment. It could look like "See: rails/rails#12069"

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

SimpleCov Report

Coverage Threshold
Line 94.27% 94%
Branch 85.8% 85%

@hannahramadan hannahramadan merged commit 74fcfe7 into dev Sep 5, 2023
25 checks passed
@hannahramadan hannahramadan deleted the rails_subscriber_documentation branch September 5, 2023 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants